use StateAtBlock and reference states when recreating missing state #277

magicxyyz · 2023-12-07T16:40:07Z

This PR is addressing following issues:

When getting state for RPCs (in the upstream code), the state getter is called first and after that the state root is referenced in hashdb dirties cache if at all. It means that the state could potentially be garbage collected in another thread and further operations on state would fail.
That is fine for normal archive node that keeps all the state and doesn't use dirties cache. It is also ok for a full node as we don't require it to keep all the state but only some recent, so RPCs are expected to fail when requesting too old state.
However, that is an issue for "hybrid" node that persists only some states and keeps recent states in dirties cache - we expect it to always be able to process RPC for any state (to emulate normal archive node functionality).
Previous implementation of recreating missing state (AdvanceStateUpToBlock), which we use for some RPCs (e.g. eth_call, etc_getBalance), accumulated changes to state in one StateDB object, what may cause some issues i.a. excessive memory usage by caches inside StateDB.

This PR:

reorders referencing and getting state, adds referencing and adds a release method to state.StateDB
changes the recreation of state to use eth.StateAtBlock which is upstream implementation for recreation of state for tracers and should be a more robust solution

tsahee

Generally looking good. Some comments.
Also - I'd like to have a test for our partial-archive-node config that will try at least some of these APIs.

eth/state_accessor.go

arbitrum/apibackend.go

magicxyyz · 2023-12-11T19:51:46Z

Also - I'd like to have a test for our partial-archive-node config that will try at least some of these APIs.

There already is a system test on nitro side, that runs this partial-archive-node, stops it, restarts it and then checks if eth_getBalance can be called for some blocks.
https://github.com/OffchainLabs/nitro/blob/a40f8f1e9ba0975452d3dc1da602f4e20142b608/system_tests/recreatestate_rpc_test.go#L429-L429

If needed, I can add some more specific tests for the changes, also in geth repo.

magicxyyz · 2023-12-19T14:20:58Z

Also - I'd like to have a test for our partial-archive-node config that will try at least some of these APIs.

There already is a system test on nitro side, that runs this partial-archive-node, stops it, restarts it and then checks if eth_getBalance can be called for some blocks. https://github.com/OffchainLabs/nitro/blob/a40f8f1e9ba0975452d3dc1da602f4e20142b608/system_tests/recreatestate_rpc_test.go#L429-L429

If needed, I can add some more specific tests for the changes, also in geth rep

As suggested, I added a specific test on nitro side for getting state for RPCs:
https://github.com/OffchainLabs/nitro/blob/bb5c908d7c16e103130e2d80c7f5fc01e4dbef2e/system_tests/recreatestate_rpc_test.go#L514C1-L557

arbitrum/apibackend.go

magicxyyz · 2024-01-12T21:18:20Z

I am still doing some testing with nitro-testnode. I have to yet confirm that, but it seems that finalizers are not good solution.

going with upstream geth and disabling fastcache in StateAtBlock

…s for full node)

…te-for-rpc

…e too long

core/state/statedb.go

tsahee · 2024-03-07T23:10:22Z

eth/state_accessor.go

@@ -70,7 +80,7 @@ func (eth *Ethereum) hashState(ctx context.Context, block *types.Block, reexec u
 			database = state.NewDatabaseWithConfig(eth.chainDb, trie.HashDefaults)
 			if statedb, err = state.New(block.Root(), database, nil); err == nil {
 				log.Info("Found disk backend for state trie", "root", block.Root(), "number", block.Number())
-				return statedb, func() { database.TrieDB().Close() }, nil
+				return statedb, noopReleaser, nil


why is that changed to noop? where will that temporarye stateDatabase be released?

Referencing here is not needed, as if the state is available it was read from disk.

I've removed some of our changes here to minimize the diff to upstream, ia. we went with how upstream solved cleans cache memory leak by just disabling the cleans cache for the temporary dbs (trie.HashDefaults has cleans = 0) => we don't need to close the db here. If there will be need for cleans cache for better performance, we can enable cleans cache - we can do that in separate PR, but if needed I can enable it in this PR.

tsahee · 2024-03-07T23:10:48Z

eth/state_accessor.go

@@ -93,7 +103,7 @@ func (eth *Ethereum) hashState(ctx context.Context, block *types.Block, reexec u
 		if !readOnly {
 			statedb, err = state.New(current.Root(), database, nil)
 			if err == nil {
-				return statedb, func() { database.TrieDB().Close() }, nil
+				return statedb, noopReleaser, nil


same question

here also if state is available then it was read from disk + same as above we don't need to close the triedb so we can remove the diff.

tsahee

LGTM

tsahee

LGTM

tsahee

I meant approve, LGTM:)

use StateAtBlock and reference states when recreating

bc55574

cla-bot bot added the s CLA signed label Dec 7, 2023

magicxyyz mentioned this pull request Dec 7, 2023

pull in geth changes for state recreation OffchainLabs/nitro#2005

Merged

fix ethapi test

0227c54

magicxyyz marked this pull request as ready for review December 8, 2023 16:18

joshuacolvin0 requested a review from tsahee December 8, 2023 19:10

tsahee reviewed Dec 9, 2023

View reviewed changes

eth/state_accessor.go Outdated Show resolved Hide resolved

eth/state_accessor.go Outdated Show resolved Hide resolved

arbitrum/apibackend.go Outdated Show resolved Hide resolved

add baseBlock comment, fix referencing befor StateAt

8d5951a

magicxyyz force-pushed the better-recreate-state-for-rpc branch from 567e6ce to 8d5951a Compare December 11, 2023 18:52

use finalizer instead of returning state release function

b158011

magicxyyz force-pushed the better-recreate-state-for-rpc branch from 814e540 to b158011 Compare December 18, 2023 16:40

clean up extra return values

8186f88

tsahee reviewed Jan 5, 2024

View reviewed changes

arbitrum/apibackend.go Outdated Show resolved Hide resolved

joshuacolvin0 requested a review from tsahee January 12, 2024 16:45

magicxyyz and others added 2 commits January 12, 2024 20:55

Merge branch 'master' into better-recreate-state-for-rpc

331c293

set statedb finalizer only in stateAndHeaderFromHeader

5645cf0

magicxyyz added 11 commits January 25, 2024 22:29

Merge branch 'master' into better-recreate-state-for-rpc

953fef0

going with upstream geth and disabling fastcache in StateAtBlock

save some states from triegc when stoping sparse archive node (same a…

ce1438b

…s for full node)

Merge branch 'master' into better-recreate-state-for-rpc

4ec21b1

Merge remote-tracking branch 'origin/master' into better-recreate-sta…

c35048f

…te-for-rpc

add debug logs with state finalizers counters

5177f70

Merge branch 'master' into better-recreate-state-for-rpc

06822bc

fix baseBlock usage in StateAtBlock

97a5103

add state release method to StateDB

65e4c8a

add todo comment

d1db8eb

fix isolation of live state database

f1fff92

arbitrum/apibackend: add live and ephemeral states metrics

e10de75

magicxyyz added 7 commits February 16, 2024 16:24

Merge branch 'master' into better-recreate-state-for-rpc

96c021a

Merge branch 'master' into better-recreate-state-for-rpc

9790d80

update state recreation metrics

cf61e26

simplify StateDB.Release, don't set finalizer as it keeps StateDB liv…

27e28ed

…e too long

bring back AdvanceStateUpToBlock

3e1f80f

cleanup debug log

af4fb22

don't panic if StateDB.SetRelease is called more then once

a2c45fb

tsahee reviewed Feb 28, 2024

View reviewed changes

core/state/statedb.go Outdated Show resolved Hide resolved

core/state/statedb.go Outdated Show resolved Hide resolved

magicxyyz and others added 3 commits March 6, 2024 13:34

bring back working finalizers

b432ef8

Merge branch 'master' into better-recreate-state-for-rpc

4e40a02

Merge branch 'master' into better-recreate-state-for-rpc

f0a0807

tsahee reviewed Mar 7, 2024

View reviewed changes

tsahee previously approved these changes Mar 8, 2024

View reviewed changes

add check for recent block in StateAndHeaderByNumberOrHash

57fcba9

magicxyyz dismissed tsahee’s stale review via 57fcba9 March 12, 2024 21:58

add comment

088149d

tsahee reviewed Mar 13, 2024

View reviewed changes

tsahee approved these changes Mar 13, 2024

View reviewed changes

tsahee merged commit e5d8587 into master Mar 13, 2024
3 checks passed

tsahee deleted the better-recreate-state-for-rpc branch March 13, 2024 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use StateAtBlock and reference states when recreating missing state #277

use StateAtBlock and reference states when recreating missing state #277

magicxyyz commented Dec 7, 2023 •

edited

Loading

tsahee left a comment

magicxyyz commented Dec 11, 2023

magicxyyz commented Dec 19, 2023

magicxyyz commented Jan 12, 2024

tsahee Mar 7, 2024

magicxyyz Mar 8, 2024

tsahee Mar 7, 2024

magicxyyz Mar 8, 2024

tsahee left a comment

tsahee left a comment

tsahee left a comment

use StateAtBlock and reference states when recreating missing state #277

use StateAtBlock and reference states when recreating missing state #277

Conversation

magicxyyz commented Dec 7, 2023 • edited Loading

tsahee left a comment

Choose a reason for hiding this comment

magicxyyz commented Dec 11, 2023

magicxyyz commented Dec 19, 2023

magicxyyz commented Jan 12, 2024

tsahee Mar 7, 2024

Choose a reason for hiding this comment

magicxyyz Mar 8, 2024

Choose a reason for hiding this comment

tsahee Mar 7, 2024

Choose a reason for hiding this comment

magicxyyz Mar 8, 2024

Choose a reason for hiding this comment

tsahee left a comment

Choose a reason for hiding this comment

tsahee left a comment

Choose a reason for hiding this comment

tsahee left a comment

Choose a reason for hiding this comment

magicxyyz commented Dec 7, 2023 •

edited

Loading